Add a promise_test sequencing clarification

[klausw]

I got bitten by a nasty race condition where a failed promise_rejects caused teardown logic to run after the next test had already started, interfering with the next test's state. Since this was unexpected, here's a proposed addition to the documentation to make this clearer. Let me know if I'm misunderstanding how this works, or if you think this should be changed in promise_test instead.

(For context, see #17898 which contains a fix to a test helper affected by this.)

[klausw]

If I'm remembering right, the sequence turned out to be something like this:

promise_test
  promise = test.step(func, ...)
    Test.prototype.step try
      promise_rejects
        test.unreached_func
      unreached_func
        step_func
          assert_unreached
            assert
              throw
    Test.prototype.step catch
      this.done()
(start next test)

As a result, if the tested promise is part of a chain, the following steps in the chain were happening after the this.done().

[jugglinmike]

How could the case fixed by gh-17898 be handled in promise_test?

[jugglinmike]

In the current (unpatched) version of the code, cleanup logic is located in a fulfillment handler for a Promise supplied to testharness.js. When the Promise rejects (e.g. for those tests that expect failure), then I wouldn't expect the fulfillment handler (and therefore the cleanup logic) to run at all.

That said, it sounds like you were seeing the cleanup code executing. Is that right?

[klausw]

How could the case fixed by gh-17898 be handled in promise_test?

The malfunctioning test in my patch was the newly added xrSession_requestReferenceSpace_features.https.html which is a promise_test via xr_promise_test and xr_session_promise_test wrappers. Since Chromium doesn't yet implement a newly added restriction in the spec, the last two tests in that file that expect a rejected promise are failing since the promise isn't being rejected.

If I'm understanding it right, the problem was that the unexpectedly not-rejected promise is indeed leading to a test failure in promise_rejects, but since the promise was successful the .then() cleanup was still being run asynchronously, and it ended up executing after the next test's start since the test harness considered the test to be in a known failed state and complete.

[klausw]

To clarify, I'm not sure what the expected behavior is if a test file contains multiple failing test. The overall state was still "Failure" as it should be, but it was very confusing to me that I couldn't consistently get test failures reported for both of the tests where I expected failures since the underlying functionality wasn't implemented yet

Depending on the order of the tests, I was either getting the second test to pass unexpectedly, or having it fail with an unrelated error due to losing its device in the middle of the test. After making the change to use add_cleanup, both fail consistently with the expected failure messages in output.

[jugglinmike]

Whether there's one failing test or multiple, the expected behavior is the same. The part that I'm trying to understand is how and why code in one test was still running after the test had failed.

Sometimes, that happens because a Promise is accidentally discarded. The current (pre-add_cleanup) version of the code does not track the Promise created by testSession.end(), but it sounds like the disconnectAllDevices operation was what you saw executing after-the-fact.

I can't explain that, though. If the harness was correctly reporting a test failure, then that means the Promise was rejected, and since disconnectAllDevices is located in a fulfillment handler, it should not have been invoked at all.

Am I interpreting your experience correctly?

[klausw]

it sounds like the disconnectAllDevices operation was what you saw executing after-the-fact.

I can't explain that, though. If the harness was correctly reporting a test failure, then that means the Promise was rejected, and since disconnectAllDevices is located in a fulfillment handler, it should not have been invoked at all.

Am I interpreting your experience correctly?

Not quite, it's confusing since this is an expected failure due to a promise that was supposed to be rejected but actually succeeded.

Specifically, the "Non-immersive session rejects local space if not requested" test calls requestReferenceSpace('local'), and according to the latest WebXR spec that promise is supposed to be rejected, but chromium doesn't implement this restriction yet, so the promise succeeds unexpectedly. The promise_rejects from makeInvalidSpaceTest notices that it didn't get rejected and fails the test, but the underlying promise is still successful and executes the chained .then().

The part I'm not entirely clear on is why the harness doesn't wait for the overall promise chain to resolve, but instead proceeds to this.done() earlier than expected. Not sure if this is a side effect of mixing promises and steps, or if it's related to xr_session_promise_test using a nested new Promise internally. That internal promise is resolved by the time this.done() runs, and I thought that this should cause the overall chain to wait for it, but I'm admittedly fuzzy on the details of how this works.

[klausw]

FYI, I initially had the wrong promise-returning call in the previous comment, it's supposed to be requestReferenceSpace('local'), done after a successful requestSession('inline, {}). Edited above, but adding as a comment also since github email doesn't reflect edits.

See also my earlier comment #17924 (comment) where I tried to trace the execution sequence - please take that with a grain of salt since I'm not familiar with the code and tend to get confused by async programming.

[sideshowbarker]

Minor copy-editing nits

[jugglinmike]

After a bit of experimentation, it doesn't look like there's much we can do about this in the framework itself.

I generally recommend reserving APIs like done and step for tests declared with async_test and relying on Promises for asynchronous control flow in tests declared with promise_test. That helps avoid ambiguities like this.

My initial thought was to enforce the recommendation in code by rejecting tests which mix paradigms. This doesn't appear to be feasible because many utilities (e.g. EventWatcher) use the async_test methods internally.

A more lax solution might be to wait for the promise_test to settle even after done has been invoked. That seems likely to cause a lot of existing tests to time out, though.

So I think we'll have to settle (Promise humor) for documentation.

[jugglinmike]

However, be aware that a test is considered finished as soon as its status is resolved, and a failed test's chained actions may still be in progress when the next test starts.

The term "status" has a specific meaning in testharness.js, so using it to describe a different concept may confuse readers. The term "actions" isn't defined for the web platform or for testharness.js. Rather than improve the wording, I suggest removing the sentence. The second sentence is what's important for test authors.

[klausw]

jugglinmike@ wrote:

I generally recommend reserving APIs like done and step for tests declared with async_test and relying on Promises for asynchronous control flow in tests declared with promise_test. That helps avoid ambiguities like this.

Ah, so was part of the issue that my makeSpaceTest used t.step()? I got the impression that promise_test's internal use of step ended up triggering the earlier-than-expected done() call.

The term "status" has a specific meaning in testharness.js, so using it to describe a different concept may confuse readers. The term "actions" isn't defined for the web platform or for testharness.js. Rather than improve the wording, I suggest removing the sentence. The second sentence is what's important for test authors.

Removed, though I think the result looks a bit odd - I'd intuitively interpret "finished" from the first sentence as being equivalent to "settled", and if that's not the case I think it would be helpful to say so explicitly. Yes, following the instructions would avoid issues, but people may still end up with an incorrect mental model.

I'm OK proceeding as-is with the suggested changes incorporated, but how would you feel about something like this?

  ... don't start running until after the previous Promise Test finishes. 
+ However, a failing test can finish while chained promises are not yet settled.
  Use [add_cleanup](#cleanup) to register ...

  ... don't start running until after the previous Promise Test finishes. 
+ However, a failing test does not wait for the overall `promise_test` to settle, so 
+ code in a `catch`/`finally` branch may run concurrently with the next test.
  Use [add_cleanup](#cleanup) to register ...

[jugglinmike]

This behavior isn't limited to failing tests. Authors risk interleaved execution any time the done method is invoked prior to Promise settling, e.g.

promise_test((t) => {
  t.done();
  return Promise.resolve();
});

We could explain this more thoroughly, but I still think the intricacies will be more distracting than helpful for the first-time contributor. As a middle ground, we could hand-wave the details and reference this discussion.

  ... don't start running until after the previous Promise Test finishes. 
+ [Under rare
+ circumstances](https://github.com/web-platform-tests/wpt/pull/17924), the
+ next test may begin to execute before the returned promise has settled.
  Use [add_cleanup](#cleanup) to register ...

That makes the finer points available to those who care and avoids confusing those who are looking for direct instruction. What do you think?

This change is included in cf227e2

[klausw]

We could explain this more thoroughly, but I still think the intricacies will be more distracting than helpful for the first-time contributor. As a middle ground, we could hand-wave the details and reference this discussion. [...]
That makes the finer points available to those who care and avoids confusing those who are looking for direct instruction. What do you think?

That sounds fine with me, I incorporated that suggestion. Thanks!

[jugglinmike]

Great!